Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove lambdas from Arc that are used at startup #45741

Merged
merged 1 commit into from
Jan 22, 2025
Merged

Remove lambdas from Arc that are used at startup #45741

merged 1 commit into from
Jan 22, 2025
+72 −19

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Jan 21, 2025

These have a small effect on startup time, so
we let's remove them without losing any functionality

Before this change, a flamegraph of hello world REST application looks like:

old

while after applying it, we have:

new

@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Jan 21, 2025
gsmet
gsmet reviewed Jan 21, 2025
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I added a few comments.

independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/ComputingCache.java Outdated Show resolved Hide resolved
independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/EventImpl.java Outdated
Comment on lines 324 to 329
predicate = predicate.and(new Predicate<>() {
@Override
public boolean test(ObserverMethod<? super T> observerMethod) {
return isNotTxObserver(observerMethod);
}
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you generate a bit more allocations this way. I would expect notify() to be called quite often?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I drop the method reference is that it brings some overhead at startup. I doubt the allocations make much of a difference with this change, but I could always check the flamegraph

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wouldn't have mentioned it but I wonder if it might not be a hot path if you have a lot of events.

@mkouba would know better.

mkouba
mkouba reviewed Jan 21, 2025
View reviewed changes
Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know that I don't quite understand your "holy war" against lambdas ;-), but if really necessary pls use cached predicates (static final fields) where possible - see my comments.

independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/EventImpl.java Outdated Show resolved Hide resolved
independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/EventImpl.java Outdated Show resolved Hide resolved
independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/EventImpl.java Outdated Show resolved Hide resolved
independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/ComputingCache.java Outdated
for (LazyValue<V> value : map.values()) {
action.accept(value.get());
}
map.forEach(new BiConsumer<K, LazyValue<V>>() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious how much better this appoach could be. From what I see in the ConcurrentHashMap the ValuesView returned from the values() is cached and here you create a new BiConsumer for every invocation. There is also no lambda here...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this wasn't meant to remove lambdas, it was meant to take advantage of the better access that forEach has for maps. But I'll remove it as to not confuse the scope of the PR

@geoand
Copy link
Contributor Author

geoand commented Jan 21, 2025

You know that I don't quite understand your "holy war" against lambdas ;-

This attritional battle is now a lot more targeted and I only do it for places that I have hard evidence based on the startup flamegraphs

@mkouba
Copy link
Contributor

mkouba commented Jan 21, 2025

You know that I don't quite understand your "holy war" against lambdas ;-

This attritional battle is now a lot more targeted and I only do it for places that I have hard evidence based on the startup flamegraphs

I'd be curious about the specific number of milliseconds this particular change results in...

@geoand
Copy link
Contributor Author

geoand commented Jan 21, 2025

I'd be curious about the specific number of milliseconds this particular change results in...

None. But if all the changes are added up, they do make a difference (more from @franz1981 on that soon)

@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 21, 2025
@mkouba
Copy link
Contributor

mkouba commented Jan 21, 2025

I'd be curious about the specific number of milliseconds this particular change results in...

None. But if all the changes are added up, they do make a difference (more from @franz1981 on that soon)

Can't wait to hear about it!

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@geoand
Copy link
Contributor Author

geoand commented Jan 21, 2025 via email

@geoand
Remove lambdas from Arc that are used at startup
6adeeda 
These have a small effect on startup time, so
we let's remove them without losing any functionality
@geoand
Copy link
Contributor Author

geoand commented Jan 21, 2025

Should be fixed now

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Jan 22, 2025 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 6adeeda.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 integration-tests/observability-lgtm

✖ io.quarkus.observability.test.LgtmReloadTest.testReload - History

  • io.quarkus.builder.BuildException: Build failure: Build failed due to errors [error]: Build step io.quarkus.observability.deployment.ObservabilityDevServiceProcessor\#startContainers threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/grafana/otel-lgtm:0.8.2 at io.quarkus.observability.deployment.ObservabilityDevServiceProcessor.lambda$startContainers$3(ObservabilityDevServiceProcessor.java:170) at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1708) at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:762) - java.lang.RuntimeException
java.lang.RuntimeException: 
io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.observability.deployment.ObservabilityDevServiceProcessor#startContainers threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/grafana/otel-lgtm:0.8.2
	at io.quarkus.observability.deployment.ObservabilityDevServiceProcessor.lambda$startContainers$3(ObservabilityDevServiceProcessor.java:170)
	at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1708)
	at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:762)
	at io.quarkus.observability.deployment.ObservabilityDevServiceProcessor.startContainers(ObservabilityDevServiceProcessor.java:113)
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:733)

@geoand geoand merged commit f1430d8 into quarkusio:main Jan 22, 2025
53 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.19 - main milestone Jan 22, 2025
@geoand geoand deleted the arc-notify branch January 22, 2025 10:41
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsmet gsmet gsmet left review comments

@mkouba mkouba mkouba approved these changes

Assignees
No one assigned
Labels
area/arc Issue related to ARC (dependency injection) triage/flaky-test
Projects
None yet
Milestone
3.19 - main
Development

Successfully merging this pull request may close these issues.
3 participants
@geoand @mkouba @gsmet